Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ctb): Pass msg.sender to dispute games from the factory #10149

Merged
merged 4 commits into from
Apr 15, 2024

Conversation

clabby
Copy link
Member

@clabby clabby commented Apr 15, 2024

Overview

Passes msg.sender from the factory to the dispute game as an immutable argument. This change allows for smart wallets to participate in the protocol as first-class citizens.

Metadata
closes https://github.com/ethereum-optimism/client-pod/issues/732
closes https://github.com/ethereum-optimism/client-pod/issues/731

@clabby clabby requested a review from a team as a code owner April 15, 2024 15:04
@clabby clabby requested review from refcell and removed request for a team April 15, 2024 15:04
@clabby clabby force-pushed the cl/ctb/pass-msg-sender-dgf branch from a28a64b to d10095b Compare April 15, 2024 15:17
Base automatically changed from inphi/fix-rez to develop April 15, 2024 19:18
@clabby clabby force-pushed the cl/ctb/pass-msg-sender-dgf branch from 64dedc0 to 909d996 Compare April 15, 2024 19:39
Copy link
Contributor

coderabbitai bot commented Apr 15, 2024

Walkthrough

Walkthrough

The updates encompass a version increment and modifications across several smart contracts and interfaces within the contracts-bedrock package, focusing on dispute management. Key changes include the addition of new functions, renaming of existing ones, and adjustments in data handling and error definitions to enhance functionality and error clarity in dispute games.

Changes

File Path Changes Summary
.../src/dispute/DisputeGameFactory.sol Updated version to "0.5.0", modified clone function to include msg.sender.
.../src/dispute/FaultDisputeGame.sol Updated version, adjusted data reading, added new functions, and modified calldata size check.
.../src/dispute/interfaces/IDisputeGame.sol Renamed function, added new getters for enhanced data access.
.../src/dispute/interfaces/IFaultDisputeGame.sol Removed l1Head() function.
.../src/libraries/DisputeErrors.sol Renamed error for clarity in data validation.
.../test/dispute/FaultDisputeGame.t.sol Updated test logic and error handling to align with changes in data validation.

Recent Review Details

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 2d354ac and 909d996.
Files ignored due to path filters (7)
  • op-bindings/bindings/disputegamefactory.go is excluded by !op-bindings/bindings/**
  • op-bindings/bindings/disputegamefactory_more.go is excluded by !op-bindings/bindings/**
  • op-bindings/bindings/faultdisputegame.go is excluded by !op-bindings/bindings/**
  • op-bindings/bindings/faultdisputegame_more.go is excluded by !op-bindings/bindings/**
  • packages/contracts-bedrock/semver-lock.json is excluded by !**/*.json
  • packages/contracts-bedrock/snapshots/abi/FaultDisputeGame.json is excluded by !**/*.json
  • packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGame.json is excluded by !**/*.json
Files selected for processing (6)
  • packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol (2 hunks)
  • packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol (5 hunks)
  • packages/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol (1 hunks)
  • packages/contracts-bedrock/src/dispute/interfaces/IFaultDisputeGame.sol (1 hunks)
  • packages/contracts-bedrock/src/libraries/DisputeErrors.sol (1 hunks)
  • packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol (4 hunks)
Additional Context Used
Learnings (1)
packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol (1)
User: clabby"
PR: ethereum-optimism/optimism#8351
File: packages/contracts-bedrock/test/dispute/OutputBisectionGame.t.sol:197-200
Timestamp: 2023-12-04T18:04:13.562Z
Learning: The user has indicated that the manual manipulation of storage in tests is an accepted strategy due to the stability of the storage layout and the complexity of introspecting the contract artifact.
Additional comments not posted (14)
packages/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol (4)

33-37: Renaming gameType to gameCreator and its updated functionality align well with the PR's objectives.


38-39: Addition of rootClaim function enhances data handling capabilities in dispute games.


43-46: Introduction of l1Head function supports better traceability of the dispute game's creation context.


49-49: Addition of extraData function enhances the flexibility of dispute games by allowing additional data association.

packages/contracts-bedrock/src/libraries/DisputeErrors.sol (1)

39-40: Renaming ExtraDataTooLong to BadExtraData improves clarity and specificity in error reporting.

packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol (2)

26-27: Update of semantic version to "0.5.0" is appropriate given the significant changes in contract functionalities.


106-116: Inclusion of msg.sender in the clone function enhances traceability and aligns with the PR's objectives to treat smart wallets as first-class entities.

Verification successful

The changes to the clone function in DisputeGameFactory.sol have been correctly implemented, and there are no outdated references in other Solidity files. The inclusion of msg.sender in the clone function is verified as per the PR's objectives.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for any remaining references to the old `clone` function signature in the codebase.
rg --type sol 'clone\(.*\)'

Length of output: 57



Script:

#!/bin/bash
# Search for any remaining references to the old `clone` function signature in the codebase without specifying file type.
rg 'clone\(.*\)'

Length of output: 804

packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol (6)

91-92: Updated version constant to "0.12.0" aligns with the PR's objective to enhance the dispute game mechanism.


449-452: The gameCreator function has been added to retrieve the address of the game creator from calldata. This is a new feature that supports the PR's goal of treating smart wallets as first-class entities.


456-457: The rootClaim function has been added to retrieve the root claim from calldata. This function supports the enhanced data handling capabilities mentioned in the PR summary.


460-461: The l1Head function has been added to retrieve the L1 head hash from calldata. This addition is part of the adjustments to improve data management in the dispute game mechanism.


468-468: The extraData function has been adjusted to retrieve 32 bytes of extra data starting from the second word within the calldata. Verify that the offset 0x54 and length 0x20 are correctly used to fetch the intended extra data.

Verification successful

The search results confirm that the offset 0x54 and length 0x20 are used in the FaultDisputeGame.sol file to fetch extra data, as specified in the review comment. This is the only occurrence found in the codebase, which aligns with the intended usage described in the review comment. Therefore, the implementation of the extraData_ fetching mechanism using these parameters is verified to be correct.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the offset 0x54 and length 0x20 are correctly used to fetch extra data across the codebase.
rg --type solidity $'0x54, 0x20'

Length of output: 66



Script:

#!/bin/bash
# Verify that the offset 0x54 and length 0x20 are correctly used to fetch extra data across the codebase.
rg '0x54, 0x20' *.sol

Length of output: 71



Script:

#!/bin/bash
# Verify that the offset 0x54 and length 0x20 are correctly used to fetch extra data across the codebase.
rg '0x54, 0x20' --glob '*.sol' -uu

Length of output: 143


521-537: Modified calldata size check to prevent incorrect game UUID creation. This change is crucial for ensuring the integrity of game instance creation. Verify that the expected calldata size 0x7A is consistent with the structure of the calldata used across related contracts.

packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol (1)

178-194: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [182-201]

Renamed test function and adjusted logic for _extraDataLen handling.

The renaming of the test function from testFuzz_initialize_extraDataTooLong_reverts to testFuzz_initialize_badExtraData_reverts aligns with the error name change in the contract. The logic adjustment in setting _extraDataLen to skip the exact 32 bytes length by incrementing it ensures that the test covers the scenario where the extra data is just above the valid threshold, which is a good edge case to test.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@clabby clabby enabled auto-merge April 15, 2024 19:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

@laizy
Copy link

laizy commented May 16, 2024

@smartcontracts, Sorry to bother you, but I reported this same issue in sherlock-audit/2024-02-optimism-2024-judging#140 as sherlock-audit/2024-02-optimism-2024-judging#194 and didn't receive the reward. hope you could take a look, and contact to the Sherlock team. I'm not specialized in auditing, and this is my first time participating. I joined this competition because I'm passionate about optimism. I've been studying optimism contract code for years and have learned a lot from you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants